-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: naming connect procedure #3157
Conversation
You can find the image built from this PR at
Built from 146097d |
d48b947
to
a58fb41
Compare
if not conf.relay: | ||
return err("waku relay (--relay=true) should be set when configuring staticnodes") | ||
try: | ||
await connectToNodes(node, conf.staticnodes, "static") | ||
except CatchableError: | ||
return err("failed to connect to static nodes: " & getCurrentExceptionMsg()) | ||
|
||
if dynamicBootstrapNodes.len > 0: | ||
if not conf.relay: | ||
return err( | ||
"waku relay (--relay=true) should be set when configuring dynamicBootstrapNodes" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need these conditions that I added. When I added them I thought that connectToNodes
, which called connectRelay
, were only related to Relay protocol but the function name was a misnomer
Thanks for that! Nevertheless, I think is interesting having a more descriptive proc name because |
Makes sense! I updated it to Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for it! 💯
Description
Looking deeper at the code, I noticed that the
connectRelay
procedure uses thenim-libp2p
connect
procedure that connects to a node without setting a protocol.connectRelay
was a misnomer and it's more correct to call itconnectPeer
Changes
connectRelay
toconnectPeer
Relay
being configured before callingconnectPeer
connectPeer
, add peer if it's not found in the peer store instead of looking specifically for Waku Relay's codec